Skip to content

checkNoPrivateLeaks: Do not allow types to refer to leaky aliases #2189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 4, 2017

Conversation

smarter
Copy link
Member

@smarter smarter commented Apr 4, 2017

checkNoPrivateLeaks can force a lot of things, this lead to
hard-to-reproduce issues in unpickling because we called
checkNoPrivateLeaks on the type parameters of a class before anything
in the class was indexed. We fix this by making sure that
checkNoPrivateLeaks never transforms type symbols, only term symbols,
therefore we can unpickle type parameters without forcing too many
things. tests/neg/leak-type.scala illustrates the new restriction that
this necessitates.

@OlivierBlanvillain Can you confirm that this fixes the issue for you too?

@smarter smarter requested a review from odersky April 4, 2017 17:25
@felixmulder
Copy link
Contributor

This PR is not going to be checked in the CI until we fix the signage issue of .drone.yml.sig

@smarter
Copy link
Member Author

smarter commented Apr 4, 2017

What issue? Do I just need to resign the .drone.yml?

@felixmulder
Copy link
Contributor

Yes, that looks like it's the issue.

@felixmulder
Copy link
Contributor

I'm not sure why - maybe it was how/because we restarted the server.

@smarter
Copy link
Member Author

smarter commented Apr 4, 2017

Nope, when I resign it, it's identical to the existing one.

@felixmulder
Copy link
Contributor

Put this:

eyJhbGciOiJIUzI1NiJ9.cGlwZWxpbmU6CiAgdGVzdDoKICAgIGltYWdlOiBsYW1wZXBmbC9kb3R0eTpsYXRlc3QKICAgIHB1bGw6IHRydWUKICAgIGNvbW1hbmRzOgogICAgICAtIGxuIC1zIC92YXIvY2FjaGUvZHJvbmUvc2NhbGEtc2NhbGEgc2NhbGEtc2NhbGEKICAgICAgLSBsbiAtcyAvdmFyL2NhY2hlL2Ryb25lL2l2eTIgIiRIT01FLy5pdnkyIgogICAgICAtIC4vc2NyaXB0cy91cGRhdGUtc2NhbGEtbGlicmFyeQogICAgICAtIHNidCAtSi1YbXg0MDk2bSAtSi1YWDpSZXNlcnZlZENvZGVDYWNoZVNpemU9NTEybSAtSi1YWDpNYXhNZXRhc3BhY2VTaXplPTEwMjRtIC1EZG90dHkuZHJvbmUubWVtPTQwOTZtICIke1RFU1R9IgogICAgd2hlbjoKICAgICAgYnJhbmNoOgogICAgICAgIGV4Y2x1ZGU6IGdoLXBhZ2VzCgogIGRvY3VtZW50YXRpb246CiAgICBpbWFnZTogbGFtcGVwZmwvZG90dHk6bGF0ZXN0CiAgICBwdWxsOiB0cnVlCiAgICBjb21tYW5kczoKICAgICAgLSAuL3Byb2plY3Qvc2NyaXB0cy9nZW5Eb2NzICIke1RFU1R9IiAkQk9UX1BBU1MKICAgIHdoZW46CiAgICAgIGJyYW5jaDogbWFzdGVyCgogIGdpdHRlcjoKICAgIGltYWdlOiBwbHVnaW5zL2dpdHRlcgogICAgd2hlbjoKICAgICAgYnJhbmNoOiBtYXN0ZXIKICAgICAgc3RhdHVzOiBjaGFuZ2VkCgogIHNsYWNrOgogICAgaW1hZ2U6IHBsdWdpbnMvc2xhY2sKICAgIGNoYW5uZWw6IGRvdHR5CiAgICB3aGVuOgogICAgICBicmFuY2g6IG1hc3RlcgogICAgICBzdGF0dXM6IGNoYW5nZWQKCm1hdHJpeDoKICBURVNUOgogICAgLSA7c2V0IHRlc3RPcHRpb25zIGluIExvY2FsUHJvamVjdCgiZG90dHktY29tcGlsZXIiKSArPSBUZXN0cy5Bcmd1bWVudChUZXN0RnJhbWV3b3Jrcy5KVW5pdCwgIi0tZXhjbHVkZS1jYXRlZ29yaWVzPWRvdHR5LnRvb2xzLmRvdGMuaHlkcmEuUGFyYWxsZWxUZXN0aW5nIikgO3Rlc3QgO2RvdHR5LWJpbi10ZXN0cy90ZXN0CiAgICAtIDtzZXQgdGVzdE9wdGlvbnMgaW4gTG9jYWxQcm9qZWN0KCJkb3R0eS1jb21waWxlci1ib290c3RyYXBwZWQiKSArPSBUZXN0cy5Bcmd1bWVudChUZXN0RnJhbWV3b3Jrcy5KVW5pdCwgIi0tZXhjbHVkZS1jYXRlZ29yaWVzPWRvdHR5LnRvb2xzLmRvdGMuaHlkcmEuUGFyYWxsZWxUZXN0aW5nIikgO3B1Ymxpc2hMb2NhbCA7ZG90dHktYm9vdHN0cmFwcGVkL3Rlc3QKICAgIC0gO3NldCB0ZXN0T3B0aW9ucyBpbiBMb2NhbFByb2plY3QoImRvdHR5LWNvbXBpbGVyIikgKz0gVGVzdHMuQXJndW1lbnQoVGVzdEZyYW1ld29ya3MuSlVuaXQsICItLWV4Y2x1ZGUtY2F0ZWdvcmllcz1kb3R0eS50b29scy5kb3RjLmh5ZHJhLlBhcmFsbGVsVGVzdGluZyIpIDtwYXJ0ZXN0LW9ubHktbm8tYm9vdHN0cmFwIC0tc2hvdy1kaWZmIC0tdmVyYm9zZQogICAgLSA7c2V0IHRlc3RPcHRpb25zIGluIExvY2FsUHJvamVjdCgiZG90dHktY29tcGlsZXItYm9vdHN0cmFwcGVkIikgKz0gVGVzdHMuQXJndW1lbnQoVGVzdEZyYW1ld29ya3MuSlVuaXQsICItLWV4Y2x1ZGUtY2F0ZWdvcmllcz1kb3R0eS50b29scy5kb3RjLmh5ZHJhLlBhcmFsbGVsVGVzdGluZyIpIDtwYXJ0ZXN0LW9ubHkgLS1zaG93LWRpZmYgLS12ZXJib3NlCiAgICAtIDtkb3R0eS1jb21waWxlci90ZXN0T25seSBkb3R0eS50b29scy5kb3RjLkNvbXBpbGF0aW9uVGVzdHMKICAgIC0gO3B1Ymxpc2hMb2NhbCA7ZG90dHktYm9vdHN0cmFwcGVkL3Rlc3RPbmx5IGRvdHR5LnRvb2xzLmRvdGMuQ29tcGlsYXRpb25UZXN0cwo.t3oJzP6FyUGFoqaxD3zWvxZtMHDI0dtxNmCg5U24MvE

@smarter
Copy link
Member Author

smarter commented Apr 4, 2017

@felixmulder You should be able to push to this branch an extra commit with that.

@smarter
Copy link
Member Author

smarter commented Apr 4, 2017

Or even better, make a separate PR and merge if the tests start.

@felixmulder
Copy link
Contributor

Hmm, I had a slight modification to my .drone.yml file.

But anyway, let's try to fix this using IM.

@smarter
Copy link
Member Author

smarter commented Apr 4, 2017

Fixed by restarting drone.

@felixmulder
Copy link
Contributor

Hooray, we did it!

`checkNoPrivateLeaks` can force a lot of things, this lead to
hard-to-reproduce issues in unpickling because we called
`checkNoPrivateLeaks` on the type parameters of a class before anything
in the class was indexed. We fix this by making sure that
`checkNoPrivateLeaks` never transforms type symbols, only term symbols,
therefore we can unpickle type parameters without forcing too many
things. tests/neg/leak-type.scala illustrates the new restriction that
this necessitates.
@smarter smarter force-pushed the fix/privateleak-forcing branch from 552cb55 to 6ae376a Compare April 4, 2017 18:24
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Glad it was relatively straightforward in the end.

@smarter smarter merged commit 34afd70 into scala:master Apr 4, 2017
@OlivierBlanvillain
Copy link
Contributor

@smarter that fixed it! Thanks 😄

@allanrenucci allanrenucci deleted the fix/privateleak-forcing branch December 14, 2017 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants